Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add field for span http request body to IntakeV2 #366

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Add field for span http request body to IntakeV2 #366

merged 7 commits into from
Dec 20, 2024

Conversation

JonasKunz
Copy link
Contributor

@JonasKunz JonasKunz commented Sep 11, 2024 •
edited
Loading

Part of elastic/apm-agent-java#3788.

The idea of this PR was to add a new field to Intake V2 to allow storing of HTTP client request bodies (therefore on spans instead of transactions) into the existing http.request.body.original elasticsearch field.

I initially though this would be trivial, because http.request.body.original is already populated from transactions. Because in modelpb the HTTP data structure is shared for spans and transactions, I expected that no additional changes (e.g. in apm-package) are necessary.

However, this doesn't seem to work: Even though the tests show that modelpb Http.Request.Body is correctly populated from IntakeV2 spans, http.request.body.original is not populated for spans in elasticsearch.

Some hints where to look would be great, I already extended the tests showing that the body is correctly populated for modeljson.

I tried also adding a request body to the transaction system tests in APM-server: This causes the system tests to still pass, even though they should fail because the body is not part of the approved documents. So somehow something weird is going on here or I'm totally missing something

kruskall
kruskall reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, just a minor comment.

additional question: isn't this changing the spec, should it be submitted to the apm repository ?

input/elasticapm/internal/modeldecoder/v2/model.go Outdated Show resolved Hide resolved
@JonasKunz
Copy link
Contributor Author

@kruskall Yeah I thought too that this would be good enough, but when testing this change with apm-server it simply didn't work:

However, this doesn't seem to work: Even though the tests show that modelpb Http.Request.Body is correctly populated from IntakeV2 spans, http.request.body.original is not populated for spans in elasticsearch.

That was my blocker and I had no idea on where/ how to find out why. Did you try it with apm-server?

@kruskall
Copy link
Member

@JonasKunz can you share the payload you are sending to the intake v2 api ?

@JonasKunz
Copy link
Contributor Author

here is the branch where I attempted to locally test with apm-server:
https://github.com/JonasKunz/apm-server/tree/span-request-bodies

This is the altered span test data where I added a body:
JonasKunz/apm-server@569c323#diff-e95fb8998062181ce4f1d92ecae37de7a3aeb56b620974e5a6cf6a86877d280fR8

@kruskall
Copy link
Member

Hey @JonasKunz 👋

It seems to be working fine for me.

  • go mod edit -replace github.com/elastic/apm-data=../apm-data && go mod tidy
  • make apm-server
  • `docker compose up -d (to setup ES)
  • ./apm-server -e (ensure apm-server.yml has the correct ES credentials, you can use admin:change if you are using docker compose)
  • curl -H 'Accept: application/json' -H 'Content-Type: application/x-ndjson' localhost:8200/intake/v2/events --data-binary @payload.json
  • Open kibana and look at the data, there should be a trace containing http.request.body.original: test-body (you need to select last x years)

payload.json:

{"metadata": {"user": {"domain": "ldap://abc", "id": "123", "email": "[email protected]", "username": "john"}, "process": {"ppid": 6789, "pid": 1234,"argv": ["node", "server.js"], "title": "node"}, "system": {"platform": "darwin", "hostname": "prod1.example.com", "architecture": "x64", "container": {"id": "container-id"}, "kubernetes": {"namespace": "namespace1", "pod": {"uid": "pod-uid", "name": "pod-name"}, "node": {"name": "node-name"}}}, "labels": {"tag1": "label1"}, "service": {"name": "backendspans", "language": {"version": "8", "name": "ecmascript"}, "agent": {"version": "3.14.0", "name": "elastic-node", "activation_method": "some_activation_method"}, "environment": "staging", "framework": {"version": "1.2.3", "name": "Express"}, "version": "5.1.3", "runtime": {"version": "8.0.0", "name": "node"}},"cloud":{"account":{"id":"account_id","name":"account_name"},"availability_zone":"cloud_availability_zone","instance":{"id":"instance_id","name":"instance_name"},"machine":{"type":"machine_type"},"project":{"id":"project_id","name":"project_name"},"provider":"cloud_provider","region":"cloud_region","service":{"name":"lambda"}}}}
{"span": {"trace_id": "utedif0123456789abcdef9876543210", "parent_id": "abcdef0123456789", "id": "ute4567890aaaade", "sync": true, "name": "SELECT FROM product_types", "type": "db.postgresql.query", "start": 2.83092, "duration": 3.781912, "timestamp": 1532976822281000, "stacktrace": [{ "filename": "net.js", "classname": "Core.js", "lineno": 547},{"filename": "file2.js", "lineno": 12, "post_context": [ "    ins.currentTransaction = prev", "}"]}, { "function": "onread", "abs_path": "net.js", "filename": "net.js", "lineno": 547, "library_frame": true, "vars": { "key": "value" }, "module": "some module", "colno": 4, "context_line": "line3", "pre_context": [ "  var trans = this.currentTransaction", "" ], "post_context": [ "    ins.currentTransaction = prev", "    return result"] }], "context": { "db": { "instance": "customers", "statement": "SELECT * FROM product_types WHERE user_id=?", "type": "sql", "user": "readonly_user", "link": "other.db.com", "rows_affected": 2}, "http": { "url": "http://localhost:8000", "status_code":200, "response":{"headers": { "content-type": [] }, "status_code":200,"transfer_size":30012,"encoded_body_size":356,"decoded_body_size":401}, "method": "GET", "request": {"id": "some-request-id", "body" :  "test-body"}}, ",destination": {"address": "0:0::0:1", "port": 5432, "service": {"type": "db", "name": "postgresql", "resource": "postgresql"}}, "service":{"name":"service1","agent":{"version":"2.2","name":"elastic-ruby", "ephemeral_id": "justanid"}}}}}

@JonasKunz JonasKunz marked this pull request as ready for review December 18, 2024 09:55
@JonasKunz JonasKunz requested a review from a team as a code owner December 18, 2024 09:55
@JonasKunz
Copy link
Contributor Author

@kruskall thanks for testing! I guess then something was wrong with my test setup.

@JonasKunz JonasKunz requested a review from kruskall December 18, 2024 10:37
@kruskall kruskall enabled auto-merge (squash) December 20, 2024 21:15
@kruskall kruskall merged commit 560cc4b into elastic:main Dec 20, 2024
4 checks passed
This was referenced Dec 21, 2024
Merged
Merged
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kruskall kruskall kruskall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JonasKunz @kruskall